fix(codex): preserve sequential reasoning summaries#5233
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
roboomp
added
fix
provider:openai
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
P3 — already merged: fedcd2a8781 merged #5233 into main, so there is no pending merge decision.
Two correctness regressions remain in the merged implementation: the standard text.done → part.done sequence adds a trailing blank thinking_delta, and a fallback-only item followed by an atomic item replays earlier summary text. The focused test passes but omits both sequences; the package changelog entry is also missing.
Thanks for the focused fix and regression coverage, @riverpilot.
| if (eventType === "response.reasoning_summary_part.done") { | ||
| if (this.#sequentialCutoffSummaries) return firstTokenTime; | ||
| if (this.runtime.currentItem?.type === "reasoning" && this.runtime.currentBlock?.type === "thinking") { | ||
| appendReasoningSummaryPartDone( | ||
| this.runtime.currentItem, | ||
| this.runtime.currentBlock, | ||
| stream, | ||
| output, | ||
| output.content.length - 1, | ||
| ); | ||
| const entry = this.runtime.openItemForEvent(rawEvent); | ||
| if (this.#sequentialCutoffSummaries) { | ||
| this.runtime.queueSummaryDelta(entry, "\n\n"); |
There was a problem hiding this comment.
blocking: response.reasoning_summary_part.done normally follows response.reasoning_summary_text.done (the repo's Responses server emits that exact order at openai-responses-server.ts:1114-1125). The text-done branch has already discarded the legacy deltas and emitted the atomic text, but this line then buffers a separator that #flushSummaryDeltas() emits at item completion. A standard delta("IGNORED") → text.done("Plan") → part.done → output_item.done stream therefore produces thinking_deltas ["Plan", "\n\n"] and final thinking "Plan\n\n", so atomic delivery is not actually authoritative. Please only queue the separator for an entry that has not received an atomic done, and cover the real event order in the regression test.
| #flushSummaryDeltas(entry: CodexOpenItem | null): void { | ||
| if (entry?.block?.type !== "thinking") return; | ||
| for (const delta of this.runtime.takeSummaryDeltas(entry)) { |
There was a problem hiding this comment.
blocking: This flush updates only the current block; it does not advance the response-global cutoffSummaries.emitted/summary state. If item 1 falls back to buffered delta "Plan" and item 2 later supplies atomic cumulative summaries "Plan" and "Plan\n\nInspect", item 2 re-emits the first section: the resulting thinking blocks are ["Plan", "Plan\n\nInspect"]. That violates the existing cross-item replay-dedup contract. Please fold fallback text into the global cutoff state (or otherwise reconcile it before later .done events) and add a mixed fallback/atomic regression case.
| /** Summary deltas buffered while waiting to see whether atomic `.done` events arrive. */ | ||
| pendingSummaryDeltas = new Map<CodexOpenItem, string[]>(); |
There was a problem hiding this comment.
should-fix: This changes observable Codex stream assembly in packages/ai, but the two-file PR omits the required packages/ai/CHANGELOG.md entry under ## [Unreleased] → ### Fixed. Please record the user-visible fix.
3 participants
Summary
.donesummaries when available and flush buffered deltas at output-item completion.Screenshots
No visual change.
Testing
bun test packages/ai/test/openai-codex-responses-lite.test.ts packages/ai/test/openai-codex-stream.test.ts packages/ai/test/openai-codex.test.tsbun run --cwd=packages/ai check:types--print-thoughtscompleted in the source worktree.Risk
Notes
reasoning.summary = "detailed"when thinking is enabled.AI Review Report
The change is narrowly scoped to sequential summary event delivery and includes a delta-only regression test.
Security Audit
No authentication, authorization, secret handling, or external request policy changes.